Skip to content

Conversation

@72MiguelGomes
Copy link
Contributor

The new version of SpringLiquibase was not released yet, but will add this PR here, and as soon as the new version is available we just need to update it and merge.

Issue: #13550.

This PR should keep on hold for a little. @isopov as soon as the new version was released please update us.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jun 30, 2018
<kotlin.version>1.2.41</kotlin.version>
<lettuce.version>5.1.0.M1</lettuce.version>
<liquibase.version>3.6.1</liquibase.version>
<liquibase.version>3.6.2-SNAPSHOT</liquibase.version>
Copy link
Contributor Author

@72MiguelGomes 72MiguelGomes Jun 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please note this was used just to test, we must remove the SNAPSHOTas soon as the new versions is available

@72MiguelGomes 72MiguelGomes force-pushed the 13550/AddSupportSpringLiquibase3.6.2 branch from 79804f7 to 61be918 Compare July 1, 2018 00:18
@philwebb philwebb added type: enhancement A general enhancement for: merge-with-amendments Needs some changes when we merge and removed status: waiting-for-triage An issue we've not yet triaged labels Jul 1, 2018
@philwebb philwebb added this to the Backlog milestone Jul 1, 2018
@isopov
Copy link
Contributor

isopov commented Jul 5, 2018

@72MiguelGomes 3.6.2 liqubiase version has been released

Copy link
Member

@snicoll snicoll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. Note that when adding new configuration keys, the appendix in the doc must be upgraded as well.

Do any of those have a default value? If so, it's a bit dangerous to override them with null. It would be nice also to expose those default values in the metadata (by initializing the field).

private String liquibaseTablespace;

/**
* Database changelog table.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

table name?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean? I am using the same that used in liquibase project

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean that the description can be more informative.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In GlobalConfiguration the description is

Name of table to use for tracking change history

which is way more informative than the Javadoc. I prefer to use that

@snicoll snicoll added the status: waiting-for-feedback We need additional information before we can continue label Jul 9, 2018
@72MiguelGomes
Copy link
Contributor Author

@snicoll thanks for the feedback, will update it asap.

@72MiguelGomes 72MiguelGomes force-pushed the 13550/AddSupportSpringLiquibase3.6.2 branch from 61be918 to 65cc3ab Compare July 12, 2018 21:01
@72MiguelGomes 72MiguelGomes force-pushed the 13550/AddSupportSpringLiquibase3.6.2 branch from 65cc3ab to 903e3ab Compare July 12, 2018 21:06
@72MiguelGomes
Copy link
Contributor Author

@snicoll Those values does not have any default value define, so there is no problem.

https://github.com/liquibase/liquibase/blob/master/liquibase-core/src/main/java/liquibase/integration/spring/SpringLiquibase.java#L81

Regarding de adoc, do you know if is possible generate that automatically during the build or is it always a manual process?

@philwebb The PR is ready to merge, please let me know if you find something that is missing.

@isopov Thanks for letting me know about liquibase release.

@snicoll
Copy link
Member

snicoll commented Jul 13, 2018

@snicoll Those values does not have any default value define, so there is no problem.

uh? Surely there must be some sort of default somewhere right? SpringLiquibase is just a wrapper, the default value is set in GlobalConfiguration.

Regarding de adoc, do you know if is possible generate that automatically during the build or is it always a manual process?

That's how it is right now, you can subscribe to #8237.

spring.liquibase.contexts= # Comma-separated list of runtime contexts to use.
spring.liquibase.default-schema= # Default database schema.
spring.liquibase.liquibase-schema= # Liquibase database schema.
spring.liquibase.liquibase-tablespace= # Liquibase tablespace.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is an indentation problem here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

@snicoll
Copy link
Member

snicoll commented Jul 13, 2018

@72MiguelGomes please don't act on the default value as I'd like to do this globally (#13765).

Can you please fix the indentation and use a better description for those new keys?

@72MiguelGomes 72MiguelGomes force-pushed the 13550/AddSupportSpringLiquibase3.6.2 branch from 5b39aa3 to f780439 Compare July 13, 2018 08:23
@snicoll snicoll changed the title Add support for new SpringLiquibase properties from 3.6.2 Upgrade to Liquibase 3.6.2 Jul 13, 2018
@snicoll snicoll self-assigned this Jul 13, 2018
@snicoll snicoll removed the status: waiting-for-feedback We need additional information before we can continue label Jul 13, 2018
@snicoll snicoll modified the milestones: Backlog, 2.1.0.M1 Jul 13, 2018
snicoll pushed a commit that referenced this pull request Jul 13, 2018
@snicoll snicoll closed this in 4c7c328 Jul 13, 2018
snicoll added a commit that referenced this pull request Jul 13, 2018
…quibase3.6.2

* pr/13625:
  Polish "Upgrade to Liquibase 3.6.2"
  Upgrade to Liquibase 3.6.2
@snicoll
Copy link
Member

snicoll commented Jul 13, 2018

@72MiguelGomes thank you, this is now merged in master.

@DanielFran
Copy link

DanielFran commented Jul 17, 2018

@snicoll Is it possible to include this to next 2.0.4 release too?

It should include those 4 PRs:
#13145
#13159
#13625
#13765

@wilkinsona
Copy link
Member

Unfortunately not. 2.0.x uses Liquibase 3.5.x and we won't upgrade to a new minor release of Liquibase in a maintenance release of Spring Boot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

for: merge-with-amendments Needs some changes when we merge type: enhancement A general enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants